[VW-355] Notification DeviceGroupMatching linking#147
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR moves notification mappings to ChangesNotification matching and inbox enrichment
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| export type DeviceGroupCandidate = { | ||
| export type DeviceGroupMatchingCandidate = { | ||
| id: string; | ||
| cpe: string[]; |
There was a problem hiding this comment.
What's your opinion of keeping cpe and adding a udi field?
The fields cpe and udi are fields on DeviceGroup, not DeviceGroupMatching.
However, the goal is to still be able to search for them in emails and create a DeviceGroupMatching that links to the DeviceGroup that has that udi/cpe strings
There may be a couple of ways to do this in SQL/prisma. One could just be making a separate query for devicegroup on cpe/udi if present, then looking to see if there's a related DeviceGroupMatching. Do you have any insight on the architecture for this?
There was a problem hiding this comment.
ended up doing the query for cpe/udi
| if(extracted.manufacturer) { | ||
| or.push({ | ||
| deviceGroupMatchings: { | ||
| some: {vendor: { OR: vendorNameOr(extracted.manufacturer)}} | ||
| } | ||
| }) | ||
| } | ||
| if(extracted.modelName) { | ||
| or.push({ | ||
| deviceGroupMatchings: { | ||
| some: { product: { OR: productNameOr(extracted.modelName)}} | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
a little concerned here, but also we don't have a ton of data to test on
but my concern is that if cveId is not present, we'll link manufacturer and model name. we'll get all vulnerabilities whose vendor/product matches, even if those vulnerabilities aren't anywhere in the original email. I think a better case would be to just lookup only cve-ID, but I could be swayed otherwise
There was a problem hiding this comment.
makes sense, removed
| continue; | ||
| // link the device group to the notification | ||
| if (decision.op === "link") { | ||
| if (!decision.targetId || !validIds.has(decision.targetId)) { |
There was a problem hiding this comment.
TODO: I would separate validIds into an object, and here do validIds.deviceGroups.has(decision.targetId).
That way the model is never able to link a vulnerability id where it was supposed to link a device group id
| // Vendor/product/version are part of a device group's identity now and | ||
| // can't be mutated in place; the only safe enrichment is unioning a new | ||
| // CPE into the existing group's cpe[] array. |
There was a problem hiding this comment.
Actions I would consider trying to take here:
- Can we update the CPE of a device group? (what you're doing now)
- Can we add a new name to nameAliases for vendor/product?
- Can we update the UDI where it didn't previously exist for a DeviceGroup?
| @@unique([notificationId, deviceGroupId]) | ||
| @@unique([workOrderTicketId, deviceGroupId], map: "ndg_mapping_workorder_devicegroup_key") | ||
| @@unique([notificationId, deviceGroupMatchingId]) | ||
| @@unique([workOrderTicketId, deviceGroupMatchingId], map: "ndg_mapping_workorder_devicegroup_key") |
|
|
||
| // find the owned DeviceGrop with the identifier first, then surface the DeviceGroupMatching | ||
| // sharing its identy as match | ||
| const identifierWhere: Prisma.DeviceGroupWhereInput[] = []; |
There was a problem hiding this comment.
this is to search with cpe/udi via DeviceGroup , then read the vendorId/productId/versionId and use it to find or create deviceGroupMatching
| select: matchingSelect | ||
| })); | ||
|
|
||
| matched.set(matching.id, { |
There was a problem hiding this comment.
because a single notification can supply both kind of evidence(a UDI and a manufacturer name) in the same extracted entry, if it is the same row, the second.set() will overwrites
| }); | ||
|
|
||
| export const extractedRemediationSchema = z.object({ | ||
| linkedCveId: z.string().regex(/^CVE-\d{4}-\d{4,}$/i).nullish(), |
There was a problem hiding this comment.
this is added for searchRemediation can search on its own
|
|
||
| export const extractedAssetSchema = z.object({ | ||
| ip: z.string().nullish(), | ||
| hostname: z.string().nullish() |
There was a problem hiding this comment.
worth to add serialNumber, macAddress?
|
|
||
| export const extractedRemediationSchema = z.object({ | ||
| linkedCveId: z.string().regex(/^CVE-\d{4}-\d{4,}$/i).nullish(), | ||
| description: z.string().nullish() |
There was a problem hiding this comment.
this is not likely getting any match realistically until fuzzy search/embedding in place
| - "create": none of the candidates match. Put the device group's identifiers in fields. Manufacturer is required to create — if you cannot supply one, prefer "link" to the closest candidate or omit the decision entirely. | ||
|
|
||
| For each extracted Vulnerability, choose exactly ONE action: | ||
| - "link": the vulnerability clearly matches an existing candidate (same CVE id). Set targetId to that candidate's id. |
There was a problem hiding this comment.
For Vulnerability, Remediation, Assets, should we also allow update/create?
There was a problem hiding this comment.
For vulnerability + remediation, yes. For asset, no (good question, that's very ambiguous behavior)
My reasoning is that an asset is something that the hospital owns, and a hospital shouldn't learn of new inventory from their emails.
Vulnerabilities + remediations can be learned from emails/integrations however
There was a problem hiding this comment.
I added the update for Vulnerability/Remediation. But for create, vulnerability has required fields userId and sarif, while remediation has userId
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
prisma/migrations/20260629161924_notification_device_group_matching/migration.sql (1)
16-17: 🩺 Stability & Availability | 🔵 TrivialNon-empty tables will fail this migration; existing mappings are dropped without backfill.
Adding
deviceGroupMatchingId TEXT NOT NULLwithout a default and droppingdeviceGroupIdwill abort the migration ifnotification_device_group_mappingalready contains rows, and any existing mappings are lost since there's no backfill from the olddeviceGroupId. Confirm this table is empty in every target environment, or add a data-migration step (create/lookup matching rows and populatedeviceGroupMatchingId) before enforcingNOT NULL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/migrations/20260629161924_notification_device_group_matching/migration.sql` around lines 16 - 17, The migration on notification_device_group_mapping will fail for any non-empty table because it drops deviceGroupId and adds deviceGroupMatchingId as NOT NULL with no backfill. Update the migration to preserve existing rows by first creating or looking up the matching records, populating deviceGroupMatchingId from the old deviceGroupId via a data-migration step, and only then enforcing NOT NULL; if this table is guaranteed empty, make that assumption explicit in the migration plan.src/lib/router-utils.ts (1)
425-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment is now stale. The comment asserts identities come from CPEs so canonicals are always CPE-backed, but
resolveMatchingIdnow acceptshasCpeand callers (e.g.match.tspassinghasCpe: false) resolve non-CPE identities. Update the comment to reflect theinput.hasCpe ?? truedefault.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/router-utils.ts` around lines 425 - 431, The doc comment on resolveMatchingId is stale because it still claims identities always come from CPEs and canonicals are CPE-backed, but the function now uses input.hasCpe ?? true and can handle non-CPE identities. Update the comment near resolveMatchingId in router-utils.ts to describe both CPE and non-CPE inputs, and note that hasCpe defaults to true when not provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/inbox/agent/match.ts`:
- Around line 103-149: The candidate formatting in match() is interpolating
null-fallback text literally because the nullish coalescing is outside the
template expression. Fix the string construction in the vulnerabilities,
remediations, and assets sections by moving each ?? fallback inside the ${...}
expression for m.description, e.description, e.hostname, and m.hostname so the
prompt renders actual fallback values instead of “??” text.
- Around line 254-324: The notification-driven enrichment path in match.ts is
still writing outside the transaction because addVendorAlias, addProductAlias,
and enrichDeviceGroupIdentifiers use the module-level prisma instead of the
active tx. Update the helper calls from enrichDeviceGroup, the update branch,
and the create branch to use the Prisma.TransactionClient so all
alias/device-group writes stay inside the same transaction as upsertMapping.
Keep the canonical resolution helpers unchanged, but thread tx through the
enrichment helpers and use it for the affected writes.
In `@src/features/inbox/server/routers.ts`:
- Around line 38-76: The inbox router helpers resolvedDeviceGroupAssetCount and
resolveDeviceGroupAssets are causing per-mapping N+1 Prisma queries and
duplicate query logic. Refactor the notification list/detail flow to batch by
distinct MatchingIdentity vendor/product pairs, fetch device groups once (or in
a few grouped queries), and resolve counts/assets in memory for each mapping.
Consolidate the shared prisma.deviceGroup.findMany scaffolding in these helpers
so the same batched query path can serve both _count and assets lookups.
In `@src/features/inbox/types.ts`:
- Around line 73-79: `NotificationDetailWithRelations` has a property-name
mismatch: the `Omit` removes `deviceGroupsMatchings`, but the reintroduced field
is named `deviceGroupMatchings`, so `deviceGroupsMatchings` is missing from the
type. Update the type alias in `NotificationDetailWithRelations` to use the
exact same key as `NotificationDetailBasePayload` and the consumer in
`notification-affected-assets-tab.tsx`, so indexed access like
`NotificationDetailWithRelations["deviceGroupsMatchings"]` resolves correctly.
In `@src/lib/router-utils.ts`:
- Around line 337-340: The early-return in resolveDeviceGroup is inverted and
prevents the common enrichment path from running. Update the guard so it returns
only when neither needsCpe nor needsUdi is true, and keep the persistence/update
branch active when either condition applies; use the existing needsCpe and
needsUdi flags in router-utils to align with the resolveDeviceGroup behavior.
---
Nitpick comments:
In
`@prisma/migrations/20260629161924_notification_device_group_matching/migration.sql`:
- Around line 16-17: The migration on notification_device_group_mapping will
fail for any non-empty table because it drops deviceGroupId and adds
deviceGroupMatchingId as NOT NULL with no backfill. Update the migration to
preserve existing rows by first creating or looking up the matching records,
populating deviceGroupMatchingId from the old deviceGroupId via a data-migration
step, and only then enforcing NOT NULL; if this table is guaranteed empty, make
that assumption explicit in the migration plan.
In `@src/lib/router-utils.ts`:
- Around line 425-431: The doc comment on resolveMatchingId is stale because it
still claims identities always come from CPEs and canonicals are CPE-backed, but
the function now uses input.hasCpe ?? true and can handle non-CPE identities.
Update the comment near resolveMatchingId in router-utils.ts to describe both
CPE and non-CPE inputs, and note that hasCpe defaults to true when not provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db3d9472-6da3-4a8d-9b74-b4ba5af932a3
📒 Files selected for processing (12)
prisma/migrations/20260629161924_notification_device_group_matching/migration.sqlprisma/schema.prismascripts/seed-notifications.tssrc/features/inbox/agent/candidate-search.tssrc/features/inbox/agent/extract.tssrc/features/inbox/agent/match.tssrc/features/inbox/components/columns.tsxsrc/features/inbox/components/notification-affected-assets-tab.tsxsrc/features/inbox/components/notification-detail.tsxsrc/features/inbox/server/routers.tssrc/features/inbox/types.tssrc/lib/router-utils.ts
| const enrichDeviceGroup = async (deviceGroupMatchingId: string, data: ReturnType<typeof cleanFields>) => { | ||
| if(!data.cpe && !data.udi) return ; | ||
| const matching = await tx.deviceGroupMatching.findUnique({ | ||
| where: {id: deviceGroupMatchingId}, | ||
| select: { vendorId: true, productId: true, versionId: true}, | ||
| }); | ||
| if(matching) { | ||
| await enrichDeviceGroupIdentifiers(matching, { cpe: data.cpe, udi: data.udi }); | ||
| } | ||
| } | ||
|
|
||
| // link the device group to the notification | ||
| if (decision.op === "link") { | ||
| if (!decision.targetId || !validIds.deviceGroupMatching.has(decision.targetId)) { | ||
| summary.skipped++; | ||
| continue; | ||
| } | ||
| await upsertMapping(decision.targetId); | ||
| await enrichDeviceGroup(decision.targetId, cleanFields(decision.fields)); | ||
| summary.linked++; | ||
| } | ||
| // update the device group and link it to the notification | ||
| // TODO: consider, we may want to separate this into 'update' and 'update_and_link' actions | ||
| else if (decision.op === "update") { | ||
| if (!decision.targetId || !validIds.deviceGroupMatching.has(decision.targetId)) { | ||
| summary.skipped++; | ||
| continue; | ||
| } | ||
| // Vendor/product/version are part of a device group's identity now and | ||
| // can't be mutated in place; the only safe enrichment is unioning a new | ||
| // CPE into the existing group's cpe[] array. | ||
| const data = cleanFields(decision.fields); | ||
|
|
||
| const targetMatching = await tx.deviceGroupMatching.findUnique({ | ||
| where: { id: decision.targetId }, | ||
| select: { versionRange: true, vendorId: true, productId: true, versionId: true } | ||
| }); | ||
|
|
||
| if(data.versionRange && targetMatching && !targetMatching.versionRange) { | ||
| await tx.deviceGroupMatching.update({ | ||
| where: { id: decision.targetId }, | ||
| data: { versionRange: data.versionRange} | ||
| }) | ||
| } | ||
| if(targetMatching) { | ||
| if(data.manufacturer) { | ||
| await addVendorAlias(targetMatching.vendorId, data.manufacturer); | ||
| } | ||
| if(data.modelName && targetMatching.productId) { | ||
| await addProductAlias(targetMatching.productId, data.modelName); | ||
| } | ||
| } | ||
| await upsertMapping(decision.targetId); | ||
| await enrichDeviceGroup(decision.targetId, data); | ||
| summary.updated++; | ||
| } else { | ||
| // create | ||
| const data = cleanFields(decision.fields); | ||
| if(!data.manufacturer){ | ||
| summary.skipped++; | ||
| continue; | ||
| } | ||
| const matchingId = await resolveMatchingId({ | ||
| vendor: data.manufacturer, | ||
| product: data.modelName, | ||
| version: data.version, | ||
| versionRange: data.versionRange, | ||
| hasCpe: false | ||
| }) | ||
| await upsertMapping(matchingId); | ||
| await enrichDeviceGroup(matchingId, data); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the helpers use the global prisma client rather than accepting a tx.
fd router-utils.ts -t f --exec rg -nP -A15 '\b(addVendorAlias|addProductAlias|enrichDeviceGroupIdentifiers|resolveMatchingId)\b\s*\(' {}Repository: PATCH-UPGRADE/viper
Length of output: 3232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/features/inbox/agent/match.ts (around transaction block) ---'
sed -n '180,360p' src/features/inbox/agent/match.ts | cat -n
echo
echo '--- src/lib/router-utils.ts (helper implementations) ---'
sed -n '180,520p' src/lib/router-utils.ts | cat -nRepository: PATCH-UPGRADE/viper
Length of output: 21812
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate prisma.$transaction usage in src/features/inbox/agent/match.ts ---'
rg -n '\$transaction|upsertMapping|resolveMatchingId|addVendorAlias|addProductAlias|enrichDeviceGroupIdentifiers' src/features/inbox/agent/match.ts
echo
echo '--- locate transaction client threading in src/lib/router-utils.ts ---'
rg -n 'TransactionClient|prisma\.' src/lib/router-utils.tsRepository: PATCH-UPGRADE/viper
Length of output: 2469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate transaction usage in src/features/inbox/agent/match.ts ---'
rg -n '\$transaction|upsertMapping|resolveMatchingId|addVendorAlias|addProductAlias|enrichDeviceGroupIdentifiers' src/features/inbox/agent/match.ts
echo
echo '--- locate module-level prisma usage in src/lib/router-utils.ts ---'
rg -n 'TransactionClient|prisma\.' src/lib/router-utils.tsRepository: PATCH-UPGRADE/viper
Length of output: 2458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/lib/router-utils.ts: resolveMatchingId and dependencies ---'
sed -n '430,520p' src/lib/router-utils.ts | cat -n
echo
echo '--- src/lib/router-utils.ts: resolveVendor / resolveProduct / resolveVersion ---'
sed -n '1,220p' src/lib/router-utils.ts | cat -nRepository: PATCH-UPGRADE/viper
Length of output: 12350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/features/inbox/agent/match.ts: full surrounding function context ---'
sed -n '1,360p' src/features/inbox/agent/match.ts | cat -n | sed -n '1,360p'Repository: PATCH-UPGRADE/viper
Length of output: 17227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- call sites for the affected helpers ---'
rg -n '\b(addVendorAlias|addProductAlias|enrichDeviceGroupIdentifiers|resolveMatchingId)\b' srcRepository: PATCH-UPGRADE/viper
Length of output: 1316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- write operations inside src/lib/router-utils.ts ---'
rg -n '\b(create|upsert|update|delete)\b' src/lib/router-utils.tsRepository: PATCH-UPGRADE/viper
Length of output: 2609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '430,485p' src/lib/router-utils.ts | cat -nRepository: PATCH-UPGRADE/viper
Length of output: 2622
Keep the notification-driven updates on tx. addVendorAlias, addProductAlias, and enrichDeviceGroupIdentifiers still write through the module-level prisma, so a later rollback can leave alias/device-group changes committed while the mapping is reverted. The canonical resolution helpers intentionally autocommit, but the enrichment path should take Prisma.TransactionClient instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/inbox/agent/match.ts` around lines 254 - 324, The
notification-driven enrichment path in match.ts is still writing outside the
transaction because addVendorAlias, addProductAlias, and
enrichDeviceGroupIdentifiers use the module-level prisma instead of the active
tx. Update the helper calls from enrichDeviceGroup, the update branch, and the
create branch to use the Prisma.TransactionClient so all alias/device-group
writes stay inside the same transaction as upsertMapping. Keep the canonical
resolution helpers unchanged, but thread tx through the enrichment helpers and
use it for the affected writes.
| async function resolvedDeviceGroupAssetCount( | ||
| matching: MatchingIdentity, | ||
| ): Promise<number> { | ||
| const candidates = await prisma.deviceGroup.findMany({ | ||
| where: deviceGroupWhereForMatching(matching), | ||
| select: { | ||
| id: true, | ||
| vendorId: true, | ||
| productId: true, | ||
| versionId: true, | ||
| version: { select: { canonicalName: true }}, | ||
| _count: { select: { assets: true }} | ||
| }, | ||
| }); | ||
| return candidates.filter((dg) => matchingAppliesToDeviceGroup(matching, dg)).reduce((sum, dg) => sum +dg._count.assets, 0); | ||
| }; | ||
|
|
||
| async function resolveDeviceGroupAssets(matching: MatchingIdentity) { | ||
| const candidates = await prisma.deviceGroup.findMany({ | ||
| where: deviceGroupWhereForMatching(matching), | ||
| select: { | ||
| id: true, | ||
| vendorId: true, | ||
| productId: true, | ||
| versionId: true, | ||
| version: { select: { canonicalName: true }}, | ||
| assets: { | ||
| select: { | ||
| id: true, | ||
| ip: true, | ||
| hostname: true, | ||
| location: true, | ||
| status: true | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| return candidates.filter((dg) => matchingAppliesToDeviceGroup(matching, dg)).flatMap((dg) => dg.assets); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Per-mapping DB round trips (N+1) on notification list/detail.
resolvedDeviceGroupAssetCount and resolveDeviceGroupAssets each issue a separate, unbounded prisma.deviceGroup.findMany (scoped only by vendor/product, no take) and are invoked once per deviceGroupsMatchings entry, per notification, inside Promise.all (Lines 126-138 for getMany, Lines 158-164 for getOne). For getMany this means up to pageSize × mappingsPerNotification extra DB queries on every inbox list load, each potentially scanning many device-group rows for a vendor before filtering in memory.
Consider batching: collect the distinct (vendorId, productId) pairs across all mappings in the page, issue one (or few) findMany calls, then resolve counts/assets per mapping from the in-memory result set.
The two helpers are also near-duplicates (same where/select scaffolding, differing only in _count vs assets selection) — worth consolidating into one query path once batched.
Also applies to: 126-138, 158-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/inbox/server/routers.ts` around lines 38 - 76, The inbox router
helpers resolvedDeviceGroupAssetCount and resolveDeviceGroupAssets are causing
per-mapping N+1 Prisma queries and duplicate query logic. Refactor the
notification list/detail flow to batch by distinct MatchingIdentity
vendor/product pairs, fetch device groups once (or in a few grouped queries),
and resolve counts/assets in memory for each mapping. Consolidate the shared
prisma.deviceGroup.findMany scaffolding in these helpers so the same batched
query path can serve both _count and assets lookups.
| // Vulnerabilities / remediations / advisories / device artifacts connect via | ||
| // DeviceGroupMatching now, not a direct relation. notificationMappings is from | ||
| // main's notifications feature and stays. | ||
| notificationMappings NotificationDeviceGroupMapping[] | ||
| // notificationMappings NotificationDeviceGroupMapping[] |
There was a problem hiding this comment.
You can just delete rather than comment out
Also I know git blame this wasn't from your code, but you can remove the comment on 283-286, I wish I had caught that from Mark's PR (it's just an obvious LLM-ism that references old code where I don't think we should)
There was a problem hiding this comment.
thanks, I missed this, should have removed instead of comment out
|
|
||
| FOR DEVICE GROUPS: A device group is a class of affected product, identified by some combination of: | ||
| - CPE string (e.g. "cpe:2.3:a:vendor:product:1.0:*:*:*:*:*:*:*") | ||
| - UDI: a device-label identifier (distinct from a CPE) |
There was a problem hiding this comment.
Worried that this instruction could confuse the model to just pick any identifier it sees
What about
- UDI: The FDA-assigned unique device identifier (e.g, "04048675556176")
^That happens to be the UDI we got in our example email for the ATlan A350: https://accessgudid.nlm.nih.gov/devices/04048675556176
| - "create": none of the candidates match. Put the device group's identifiers in fields. Manufacturer is required to create — if you cannot supply one, prefer "link" to the closest candidate or omit the decision entirely. | ||
|
|
||
| For each extracted Vulnerability, choose exactly ONE action: | ||
| - "link": the vulnerability clearly matches an existing candidate (same CVE id). Set targetId to that candidate's id. |
There was a problem hiding this comment.
For vulnerability + remediation, yes. For asset, no (good question, that's very ambiguous behavior)
My reasoning is that an asset is something that the hospital owns, and a hospital shouldn't learn of new inventory from their emails.
Vulnerabilities + remediations can be learned from emails/integrations however
| assets NotificationAssetMapping[] | ||
| deviceGroups NotificationDeviceGroupMapping[] | ||
| assets NotificationAssetMapping[] | ||
| deviceGroupsMatchings NotificationDeviceGroupMapping[] |
There was a problem hiding this comment.
Consider renaming to deviceGroupMatchings, here (not deviceGroup_s_Matchings)
And see the relevant coderabbit error here: https://github.com/PATCH-UPGRADE/viper/pull/147/changes#r3514907510
There was a problem hiding this comment.
Consider moving fns like addVendorAlias, addProductAlias, enrichDeviceGroupIdentifiers, to a new lib file (one idea: split up agent/match.ts into agent/match/index.ts and agent/match/utils.ts, but feel free to pick something else, too)
My original intention with router-utils was helper functions for trpc endpoints, that are used in multiple router.ts files
There was a problem hiding this comment.
moved them to src/features/inbox/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/inbox/agent/match.ts (1)
426-432: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve CPE presence when resolving a new matching id.
This path drops
data.cpeby always passinghasCpe: false, so a newly resolvedDeviceGroupMatchingcan be created as if no CPE was present even when the LLM extracted one.🐛 Proposed fix
const matchingId = await resolveMatchingId({ vendor: data.manufacturer, product: data.modelName, version: data.version, versionRange: data.versionRange, - hasCpe: false, + hasCpe: Boolean(data.cpe), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/inbox/agent/match.ts` around lines 426 - 432, The new matching-id resolution in match.ts is incorrectly discarding extracted CPE data by hardcoding hasCpe to false. Update the resolveMatchingId call in the device matching flow to derive hasCpe from the current data.cpe presence so newly created DeviceGroupMatching records preserve whether a CPE was extracted. Keep the fix localized to the matching logic around resolveMatchingId and the data fields already available there.
♻️ Duplicate comments (2)
src/features/inbox/agent/match.ts (2)
148-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the template fallbacks before sending candidate context to the model.
The
??fallbacks are rendered as literal text, and Line 194 shows the extracted asset serial number for every candidate instead of the candidate’sserialNumber. This corrupts the prompt context.🐛 Proposed fix
- ` - id: ${m.id} | cveId: ${m.cveId ?? "(none)"} | description: ${m.description} ?? "(none)"} | cvssScore: ${m.cvssScore ?? "(none)"} | cvssVector: ${m.cvssVector ?? "(none)"}`, + ` - id: ${m.id} | cveId: ${m.cveId ?? "(none)"} | description: ${m.description ?? "(none)"} | cvssScore: ${m.cvssScore ?? "(none)"} | cvssVector: ${m.cvssVector ?? "(none)"}`,- const line = `Remediations #${i + 1}: linkedtoCveId=${e.linkedCveId ?? "?"} | description=${e.description} ?? "?"}`; + const line = `Remediations #${i + 1}: linkedtoCveId=${e.linkedCveId ?? "?"} | description=${e.description ?? "?"}`;- const line = `Asset #${i + 1}: ip=${e.ip ?? "?"} | hostname=${e.hostname} ?? "?"} | macAddress=${e.macAddress} ?? "?"} | serialNumber=${e.serialNumber} ?? "?"}`; + const line = `Asset #${i + 1}: ip=${e.ip ?? "?"} | hostname=${e.hostname ?? "?"} | macAddress=${e.macAddress ?? "?"} | serialNumber=${e.serialNumber ?? "?"}`;- ` - id: ${m.id} | ip: ${m.ip ?? "(none)"} | hostname: ${m.hostname} ?? "(none)"} | macAddress: ${m.macAddress} ?? "(none)"} | serialNumber=${e.serialNumber} ?? "(none)"}`, + ` - id: ${m.id} | ip: ${m.ip ?? "(none)"} | hostname: ${m.hostname ?? "(none)"} | macAddress: ${m.macAddress ?? "(none)"} | serialNumber=${m.serialNumber ?? "(none)"}`,Also applies to: 165-165, 188-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/inbox/agent/match.ts` at line 148, The candidate context formatter in match.ts is inserting literal “??” text and using the extracted asset serial number for every entry instead of each candidate’s own serialNumber. Fix the template strings that build the prompt context so all fallbacks are evaluated outside the rendered text, and update the candidate serialization around the relevant context-building logic to reference the candidate’s serialNumber field rather than the extracted asset value.
350-353: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep enrichment writes on the active transaction client.
These helper calls still write through module-level
prismawhile the mapping changes usetx, so rollback can leave aliases/CPE/CVSS/asset identifiers committed without the corresponding notification mapping. Threadtxinto the enrichment helpers or move these writes outside the transaction intentionally.Also applies to: 409-414, 467-472, 531-536
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/inbox/agent/match.ts` around lines 350 - 353, The enrichment helpers are still writing through the module-level prisma instead of the active transaction client, which can leave identifier writes committed even if the notification mapping rolls back. Update the enrichment calls in match handling to use the same tx passed through the transaction flow, either by threading tx into helpers like enrichDeviceGroupIdentifiers and the other enrichment functions or by moving those writes fully outside the transaction on purpose. Make the same change wherever these helper writes occur so all alias/CPE/CVSS/asset identifier updates stay consistent with the mapping work.
🧹 Nitpick comments (2)
src/features/inbox/agent/match.ts (1)
11-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
@/*alias for src-local imports.
../utils,./candidate-search, and./extractstill reference files undersrc; switch them to the configured alias. As per coding guidelines, “Use@/* import alias consistently throughout the codebase to reference src/* files.”♻️ Proposed import cleanup
import { addProductAlias, addVendorAlias, enrichAssetIdentifiers, enrichDeviceGroupIdentifiers, enrichVulnerabilityCvss, parseCvssScore, -} from "../utils"; -import type { Candidates } from "./candidate-search"; -import type { ExtractResult } from "./extract"; +} from "`@/features/inbox/utils`"; +import type { Candidates } from "`@/features/inbox/agent/candidate-search`"; +import type { ExtractResult } from "`@/features/inbox/agent/extract`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/inbox/agent/match.ts` around lines 11 - 20, The imports in match.ts still use relative paths for src-local modules; update the imports from utils, candidate-search, and extract to use the configured `@/`* alias instead of ../ or ./ paths. Keep the existing symbols like addProductAlias, Candidates, and ExtractResult, but change their module specifiers to the aliased src/* form for consistency with the codebase import guidelines.Source: Coding guidelines
src/features/inbox/utils.ts (1)
132-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
addVendorAlias/addProductAliasare near-identical; consider a shared generic helper.Both functions differ only by the Prisma model (
vendorvsproduct). Extracting a sharedaddAlias(model, id, alias)helper would reduce duplication and prevent future divergence (similar to theneedsScoreinconsistency above).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/inbox/utils.ts` around lines 132 - 173, The alias update logic in addVendorAlias and addProductAlias is duplicated except for the Prisma model used. Extract the shared flow into a generic addAlias helper that takes the model (vendor/product) plus id and alias, and reuse the existing normalizeName, findUnique, and update logic there. Keep the model-specific wrappers if needed, but have them delegate to the shared helper so the behavior stays identical and future changes only happen in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/inbox/agent/extract.ts`:
- Around line 26-33: In extractedVulnerabilitySchema, cvssScore is currently
assigned the nullish function reference instead of a Zod schema, so update the
cvssScore chain to actually call nullish() like the other fields. Keep the fix
localized to the z.object definition in extractedVulnerabilitySchema so the
schema type-checks and constructs correctly.
In `@src/features/inbox/agent/match.ts`:
- Around line 437-476: The vulnerability and remediation branches in match.ts
currently accept decisionSchema op: "create" but still require targetId, so
create decisions get skipped silently. Update the handling in the decision
processing logic around the vulnerability and remediation cases to either
implement a real create-and-map flow for those kinds or explicitly reject create
in the schema/validation so only branches that can create support it. Use the
existing decision.kind, decision.op, targetId, and summary tracking paths to
keep the behavior consistent.
- Line 17: The `match.ts` cleanup path is still calling `parseCvssScore` on
`cvssScore`, but `cleanFields()` already produces a numeric `cvssScore` and the
schema validates its range. Update the matching logic to use the numeric
`cvssScore` directly in the relevant `match`/field-cleanup flow, and remove the
`parseCvssScore` conversion/import where it is no longer needed.
In `@src/features/inbox/utils.ts`:
- Around line 175-181: Update parseCvssScore in the inbox utilities so its
validation matches the rest of the CVSS handling: the helper should only accept
numeric scores from 0 through 10, not 20. Adjust the upper-bound check inside
parseCvssScore to align with extractedVulnerabilitySchema.cvssScore in
extract.ts, and keep the same undefined/NaN rejection behavior.
- Around line 95-108: The `needsScore` check in `updateVulnerability` treats
`undefined` as a valid score update, causing unnecessary
`prisma.vulnerability.update()` calls when no CVSS score was parsed. Align it
with the existing truthy/defined checks used by `needsVector`, `needsMac`, and
`needsSerial` by only setting `needsScore` when `updates.cvssScore` is actually
present and the current `vulnerability.cvssScore` is null, then keep the
conditional payload in the `prisma.vulnerability.update` call consistent with
that guard.
---
Outside diff comments:
In `@src/features/inbox/agent/match.ts`:
- Around line 426-432: The new matching-id resolution in match.ts is incorrectly
discarding extracted CPE data by hardcoding hasCpe to false. Update the
resolveMatchingId call in the device matching flow to derive hasCpe from the
current data.cpe presence so newly created DeviceGroupMatching records preserve
whether a CPE was extracted. Keep the fix localized to the matching logic around
resolveMatchingId and the data fields already available there.
---
Duplicate comments:
In `@src/features/inbox/agent/match.ts`:
- Line 148: The candidate context formatter in match.ts is inserting literal
“??” text and using the extracted asset serial number for every entry instead of
each candidate’s own serialNumber. Fix the template strings that build the
prompt context so all fallbacks are evaluated outside the rendered text, and
update the candidate serialization around the relevant context-building logic to
reference the candidate’s serialNumber field rather than the extracted asset
value.
- Around line 350-353: The enrichment helpers are still writing through the
module-level prisma instead of the active transaction client, which can leave
identifier writes committed even if the notification mapping rolls back. Update
the enrichment calls in match handling to use the same tx passed through the
transaction flow, either by threading tx into helpers like
enrichDeviceGroupIdentifiers and the other enrichment functions or by moving
those writes fully outside the transaction on purpose. Make the same change
wherever these helper writes occur so all alias/CPE/CVSS/asset identifier
updates stay consistent with the mapping work.
---
Nitpick comments:
In `@src/features/inbox/agent/match.ts`:
- Around line 11-20: The imports in match.ts still use relative paths for
src-local modules; update the imports from utils, candidate-search, and extract
to use the configured `@/`* alias instead of ../ or ./ paths. Keep the existing
symbols like addProductAlias, Candidates, and ExtractResult, but change their
module specifiers to the aliased src/* form for consistency with the codebase
import guidelines.
In `@src/features/inbox/utils.ts`:
- Around line 132-173: The alias update logic in addVendorAlias and
addProductAlias is duplicated except for the Prisma model used. Extract the
shared flow into a generic addAlias helper that takes the model (vendor/product)
plus id and alias, and reuse the existing normalizeName, findUnique, and update
logic there. Keep the model-specific wrappers if needed, but have them delegate
to the shared helper so the behavior stays identical and future changes only
happen in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91999ec5-a317-4c3c-8c0f-a9bfad6fc82c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.jsonprisma/migrations/20260702173451_work_order_id_device_group_matching_id/migration.sqlprisma/schema.prismascripts/seed-notifications.tssrc/features/inbox/agent/candidate-search.tssrc/features/inbox/agent/extract.tssrc/features/inbox/agent/match.tssrc/features/inbox/components/notification-affected-assets-tab.tsxsrc/features/inbox/server/routers.tssrc/features/inbox/types.tssrc/features/inbox/utils.tssrc/lib/router-utils.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/features/inbox/components/notification-affected-assets-tab.tsx
- src/features/inbox/server/routers.ts
- prisma/schema.prisma
- src/features/inbox/types.ts
- scripts/seed-notifications.ts
- src/features/inbox/agent/candidate-search.ts
| } else if (decision.kind === "vulnerability") { | ||
| if ( | ||
| !decision.targetId || | ||
| !validIds.vulnerability.has(decision.targetId) | ||
| ) { | ||
| summary.skipped++; | ||
| continue; | ||
| } | ||
| await tx.notificationVulnerabilityMapping.upsert({ | ||
| where: { | ||
| notificationId_deviceGroupId: { notificationId, deviceGroupId }, | ||
| notificationId_vulnerabilityId: { | ||
| notificationId, | ||
| vulnerabilityId: decision.targetId, | ||
| }, | ||
| }, | ||
| create: { | ||
| notificationId, | ||
| deviceGroupId, | ||
| vulnerabilityId: decision.targetId, | ||
| confidence, | ||
| reasonWhy: decision.reasonWhy, | ||
| }, | ||
| update: { confidence, reasonWhy: decision.reasonWhy }, | ||
| }); | ||
|
|
||
| // link the device group to the notification | ||
| if (decision.op === "link") { | ||
| if (!decision.targetId || !validIds.has(decision.targetId)) { | ||
| summary.skipped++; | ||
| continue; | ||
| const data = cleanFields(decision.fields); | ||
| if (decision.op === "update" && data.description) { | ||
| await tx.vulnerability.update({ | ||
| where: { id: decision.targetId }, | ||
| data: { description: data.description }, | ||
| }); | ||
| } | ||
| await upsertMapping(decision.targetId); | ||
| summary.linked++; | ||
| } | ||
| // update the device group and link it to the notification | ||
| // TODO: consider, we may want to separate this into 'update' and 'update_and_link' actions | ||
| else if (decision.op === "update") { | ||
| if (!decision.targetId || !validIds.has(decision.targetId)) { | ||
| const cvssScore = parseCvssScore(data.cvssScore); | ||
| if (cvssScore !== null || data.cvssVector) { | ||
| await enrichVulnerabilityCvss(decision.targetId, { | ||
| cvssScore, | ||
| cvssVector: data.cvssVector, | ||
| }); | ||
| } | ||
| if (decision.op === "update") summary.updated++; | ||
| else summary.linked++; | ||
| } else if (decision.kind === "remediation") { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Implement or explicitly disallow create for vulnerabilities and remediations.
decisionSchema permits op: "create", but these branches require targetId, so creates are silently skipped. Either add create-and-map flows for vulnerability/remediation or make the schema discriminated so only device-group matching can create.
Also applies to: 476-510
🧰 Tools
🪛 GitHub Check: Lint, TypeScript Type, & Dependency Check
[failure] 467-467:
Argument of type 'number | undefined' is not assignable to parameter of type 'string | undefined'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/inbox/agent/match.ts` around lines 437 - 476, The vulnerability
and remediation branches in match.ts currently accept decisionSchema op:
"create" but still require targetId, so create decisions get skipped silently.
Update the handling in the decision processing logic around the vulnerability
and remediation cases to either implement a real create-and-map flow for those
kinds or explicitly reject create in the schema/validation so only branches that
can create support it. Use the existing decision.kind, decision.op, targetId,
and summary tracking paths to keep the behavior consistent.
| const needsScore = | ||
| updates.cvssScore !== null && vulnerability.cvssScore === null; | ||
| const needsVector = !!updates.cvssVector && !vulnerability.cvssVector; | ||
|
|
||
| if (!needsScore && !needsVector) return; | ||
|
|
||
| await prisma.vulnerability.update({ | ||
| where: { id: vulnerabilityId }, | ||
| data: { | ||
| ...(needsScore ? { cvssScore: updates.cvssScore } : {}), | ||
| ...(needsVector ? { cvssVector: updates.cvssVector } : {}), | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
needsScore guard mishandles undefined vs null.
updates.cvssScore !== null is true even when updates.cvssScore is undefined (the common "no CVSS parsed" case), so whenever the existing DB score is null, needsScore becomes true and triggers a no-op prisma.vulnerability.update() call with cvssScore: undefined in the payload. This is inconsistent with the truthy-based pattern used in needsVector, needsMac, and needsSerial just below/above.
🐛 Proposed fix
- const needsScore =
- updates.cvssScore !== null && vulnerability.cvssScore === null;
+ const needsScore =
+ updates.cvssScore != null && vulnerability.cvssScore === null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const needsScore = | |
| updates.cvssScore !== null && vulnerability.cvssScore === null; | |
| const needsVector = !!updates.cvssVector && !vulnerability.cvssVector; | |
| if (!needsScore && !needsVector) return; | |
| await prisma.vulnerability.update({ | |
| where: { id: vulnerabilityId }, | |
| data: { | |
| ...(needsScore ? { cvssScore: updates.cvssScore } : {}), | |
| ...(needsVector ? { cvssVector: updates.cvssVector } : {}), | |
| }, | |
| }); | |
| } | |
| const needsScore = | |
| updates.cvssScore != null && vulnerability.cvssScore === null; | |
| const needsVector = !!updates.cvssVector && !vulnerability.cvssVector; | |
| if (!needsScore && !needsVector) return; | |
| await prisma.vulnerability.update({ | |
| where: { id: vulnerabilityId }, | |
| data: { | |
| ...(needsScore ? { cvssScore: updates.cvssScore } : {}), | |
| ...(needsVector ? { cvssVector: updates.cvssVector } : {}), | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/inbox/utils.ts` around lines 95 - 108, The `needsScore` check in
`updateVulnerability` treats `undefined` as a valid score update, causing
unnecessary `prisma.vulnerability.update()` calls when no CVSS score was parsed.
Align it with the existing truthy/defined checks used by `needsVector`,
`needsMac`, and `needsSerial` by only setting `needsScore` when
`updates.cvssScore` is actually present and the current
`vulnerability.cvssScore` is null, then keep the conditional payload in the
`prisma.vulnerability.update` call consistent with that guard.
| export function parseCvssScore(value: string | undefined): number | undefined { | ||
| if (!value) return undefined; | ||
| const parsed = Number(value); | ||
| return Number.isFinite(parsed) && parsed >= 0 && parsed <= 20 | ||
| ? parsed | ||
| : undefined; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
CVSS score upper bound should be 10, not 20.
parseCvssScore accepts scores up to 20, but CVSS scores (v2/v3/v4) never exceed 10.0, and extractedVulnerabilitySchema.cvssScore in extract.ts enforces .max(10). This upper-bound mismatch lets invalid scores (10–20) slip through this helper while extraction would have already rejected them, creating inconsistent validation surfaces.
🐛 Proposed fix
return Number.isFinite(parsed) && parsed >= 0 && parsed <= 20
+ return Number.isFinite(parsed) && parsed >= 0 && parsed <= 10
? parsed
: undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function parseCvssScore(value: string | undefined): number | undefined { | |
| if (!value) return undefined; | |
| const parsed = Number(value); | |
| return Number.isFinite(parsed) && parsed >= 0 && parsed <= 20 | |
| ? parsed | |
| : undefined; | |
| } | |
| export function parseCvssScore(value: string | undefined): number | undefined { | |
| if (!value) return undefined; | |
| const parsed = Number(value); | |
| return Number.isFinite(parsed) && parsed >= 0 && parsed <= 10 | |
| ? parsed | |
| : undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/inbox/utils.ts` around lines 175 - 181, Update parseCvssScore in
the inbox utilities so its validation matches the rest of the CVSS handling: the
helper should only accept numeric scores from 0 through 10, not 20. Adjust the
upper-bound check inside parseCvssScore to align with
extractedVulnerabilitySchema.cvssScore in extract.ts, and keep the same
undefined/NaN rejection behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/inbox/utils.ts (2)
152-173: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass the transaction client through these helpers.
applyDecisions()calls them insideprisma.$transaction(...), but each helper still uses the module-levelprismaclient, so the alias/identifier writes can commit outside the rollback boundary if a later decision fails. ThreadtxthroughaddVendorAlias,addProductAlias,enrichDeviceGroupIdentifiers,enrichVulnerabilityCvss, andenrichAssetIdentifiers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/inbox/utils.ts` around lines 152 - 173, The helper functions used by applyDecisions() still read and write through the module-level prisma client instead of the transaction client, so update addProductAlias and the related helpers addVendorAlias, enrichDeviceGroupIdentifiers, enrichVulnerabilityCvss, and enrichAssetIdentifiers to accept a tx parameter and use it for all Prisma calls. Then thread the transaction from applyDecisions() into each helper so alias/identifier updates stay inside prisma.$transaction() and roll back together if any decision fails.
44-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard
cvssScoreagainstundefined.!== nullstill treats an omitted score as present, so this path can emit no-op vulnerability updates when the score is absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/inbox/utils.ts` around lines 44 - 151, `enrichVulnerabilityCvss` is treating an omitted `cvssScore` as actionable because the `!== null` check also passes for `undefined`. Update the `needsScore` logic in this function to require a real score value before preparing the `prisma.vulnerability.update` payload, while still preserving the existing “only fill missing data” behavior for `cvssVector`.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/features/inbox/utils.ts`:
- Around line 152-173: The helper functions used by applyDecisions() still read
and write through the module-level prisma client instead of the transaction
client, so update addProductAlias and the related helpers addVendorAlias,
enrichDeviceGroupIdentifiers, enrichVulnerabilityCvss, and
enrichAssetIdentifiers to accept a tx parameter and use it for all Prisma calls.
Then thread the transaction from applyDecisions() into each helper so
alias/identifier updates stay inside prisma.$transaction() and roll back
together if any decision fails.
- Around line 44-151: `enrichVulnerabilityCvss` is treating an omitted
`cvssScore` as actionable because the `!== null` check also passes for
`undefined`. Update the `needsScore` logic in this function to require a real
score value before preparing the `prisma.vulnerability.update` payload, while
still preserving the existing “only fill missing data” behavior for
`cvssVector`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4288444f-290e-482c-b1cc-dd4e44322d37
📒 Files selected for processing (3)
src/features/inbox/agent/extract.tssrc/features/inbox/agent/match.tssrc/features/inbox/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/inbox/agent/extract.ts
- src/features/inbox/agent/match.ts
2 participants
Jira issue: https://northeastern-patch.atlassian.net/browse/VW-355
Summary by CodeRabbit